[codex] Port Reborn Responses API input handling - #5347
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates OpenAI Responses compatibility for ChangesLocal-dev external tool capabilities
OpenAI Responses compatibility
Reborn WebUI smoke test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces structured context injection (x_context) to the OpenAI-compatible Responses API, including size validation, custom deserialization for input items, and comprehensive integration tests. The review feedback suggests fixing a deserialization edge case where a missing role field on a message item produces a misleading error, and recommends performance optimizations to avoid unnecessary heap allocations during context size validation and string formatting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
🚅 Deployed to the ironclaw-pr-5347 environment in ironclaw-ci-preview
|
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs (1)
366-381: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject external tools that reuse an existing provider tool name.
This only checks
safe_name/capability_id. If an external tool’sProviderToolNamematches an existing host/extension provider tool name,capability_id_for_tool_name()resolves the external map first and can route a model call to the external-tool parking path instead of the intended host capability.Fail closed on provider tool-name collisions
+ let existing_provider_tool_names = self + .inner_tool_definitions()? + .into_iter() + .map(|definition| definition.name) + .collect::<Vec<_>>(); for spec in specs { @@ let provider_tool_name = provider_tool_name_for_external_tool(spec.name())?; - let capability_id = external_tool_capability_id(provider_tool_name.as_str())?; + if existing_provider_tool_names + .iter() + .any(|name| name == &provider_tool_name) + || capability_ids_by_tool_name.contains_key(&provider_tool_name) + { + return Err(AgentLoopHostError::new( + AgentLoopHostErrorKind::InvalidInvocation, + "external tool conflicts with another provider tool name", + )); + } + let capability_id = spec.capability_id().clone();As per coding guidelines, local capability routing must fail closed for trust/approval-sensitive surfaces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs` around lines 366 - 381, The collision check in external_tool_capability handling only compares capability_id/safe_name, so an external tool can still reuse an existing ProviderToolName and hijack routing. Update the conflict validation near provider_tool_name_for_external_tool, external_tool_capability_id, and capability_ids_by_tool_name to also reject any external ProviderToolName that already exists in the host/extension provider-tool registry, and fail closed with InvalidInvocation before inserting the new ToolSpec.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/families/mod.rs`:
- Around line 55-64: The `default_with_iteration_limit` helper in
`families::mod` overrides the planner budget via `DefaultPlanner::with_budget()`
but still reuses the original `id()` and `version()`, so this variant publishes
the default `LoopFamily` identity while behaving differently. Recompute or
explicitly override the `ComponentIdentity` after applying the
`DefaultBudgetStrategy`, and ensure `LoopFamily::new` is given an
identity/version that matches the iteration-limited variant instead of the
default planner’s values.
In
`@crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs`:
- Around line 453-457: Preserve the specific ExternalToolCatalogError kind in
catalog_error instead of mapping every case to
AgentLoopHostErrorKind::Unavailable. Update catalog_error in
external_tool_capability.rs to match on ironclaw_turns::ExternalToolCatalogError
and keep InvalidRegistration distinct from true catalog outages, while still
constructing an AgentLoopHostError with the appropriate kind and message. Use
the existing catalog_error function and AgentLoopHostError::new so the
underlying cause is not collapsed into a generic infrastructure failure.
- Around line 116-127: The capability id generation in
external_tool_capability_id is duplicating the naming logic instead of using the
catalog-owned source of truth. Update the code path that builds the external
tool capability to read the canonical capability_id already validated and stored
on ExternalToolSpec, and remove the local sanitizer-based recomputation so the
runtime stays aligned with ironclaw_turns and any future catalog changes.
In `@crates/ironclaw_reborn_openai_compat/src/responses.rs`:
- Around line 63-108: The deserialization match in responses.rs is
misclassifying typed message items because the `if object.contains_key("role")`
guard applies to both `Some("message")` and `None`, so a `type: "message"`
object without `role` falls through to the unsupported-type branch. Update the
match around `Self::deserialize` so `Some("message")` always deserializes
through `MessageWire`, letting field validation report the missing
`role`/`content` error, while keeping the `None` fallback for untyped role-based
messages.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py`:
- Around line 157-173: The raw SSE test is using httpx.stream and manually
parsing event frames, but the e2e harness requires aiohttp for SSE streaming.
Update test_reborn_legacy_responses_streaming_raw_sse to use the shared
sse_stream() helper from helpers.py instead of response.aiter_lines() so raw
SSE/keepalive behavior matches the rest of the suite. Keep the REST client usage
on httpx.AsyncClient, and preserve the existing assertions for response.created
and response.completed.
---
Outside diff comments:
In
`@crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs`:
- Around line 366-381: The collision check in external_tool_capability handling
only compares capability_id/safe_name, so an external tool can still reuse an
existing ProviderToolName and hijack routing. Update the conflict validation
near provider_tool_name_for_external_tool, external_tool_capability_id, and
capability_ids_by_tool_name to also reject any external ProviderToolName that
already exists in the host/extension provider-tool registry, and fail closed
with InvalidInvocation before inserting the new ToolSpec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 894b73f8-72a1-4e83-8903-92cd312c1d36
📒 Files selected for processing (13)
crates/ironclaw_agent_loop/src/families/mod.rscrates/ironclaw_reborn/src/app_loop_family.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_openai_compat/src/responses.rscrates/ironclaw_reborn_openai_compat/src/responses_workflow.rscrates/ironclaw_reborn_openai_compat/tests/responses_workflow_handlers_contract.rstests/e2e/helpers.pytests/e2e/mock_llm.pytests/e2e/reborn_webui_harness.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs (1)
891-896: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn the canonical wire name
contextinerror.param.This validator sits on the public
/api/v1/responsesboundary, but it reports"x_context"when the request field is exposed ascontext. That leaks the Rust-only field name to clients and points callers at a parameter they never sent. Please return the canonical wire name here and cover it in the handler contract test. As per coding guidelines,At channel boundaries to users, map internal errors to sanitized messages, and the line-range change details state that this field serializes under thecontextalias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs` around lines 891 - 896, The Responses request validator is returning the Rust-only parameter name instead of the public wire name. Update the `responses_workflow` validation path that builds `OpenAiCompatHttpError::invalid_request` for `request.x_context` so `error.param` uses the canonical `context` name, and make sure the corresponding handler contract test asserts the exposed parameter name is `context` rather than `x_context`.Source: Coding guidelines
tests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py (1)
167-183: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert a streaming-only event, not just terminal lifecycle events.
This now passes even if
/v1/responsesemits onlyresponse.createdandresponse.completed. The compat streaming contract also emits text events, so this caller-level E2E should require at leastresponse.output_text.delta/response.output_text.doneand keep the completion ordering check.Suggested tightening
assert events assert "response.created" in events + assert "response.output_text.delta" in events + assert "response.output_text.done" in events assert "response.completed" in events + assert events.index("response.created") < events.index("response.completed")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py` around lines 167 - 183, The E2E check in the response-stream parsing loop is too weak because it only asserts terminal lifecycle events; tighten it in the streaming assertion around the events collection so it also requires a streaming-only text event such as response.output_text.delta or response.output_text.done, while still keeping the existing response.created and response.completed ordering checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_openai_compat/src/responses.rs`:
- Around line 63-76: The deserialization in responses.rs is treating a present
non-string "type" the same as an absent one, which lets malformed payloads fall
back to the legacy role-based Message path. Update the matching logic in the
OpenAiResponsesInputItem deserializer to inspect object.get("type") directly so
only a truly missing type can use the legacy branch, and reject any non-string
type value before reaching the fallback. Keep the existing MessageWire /
OpenAiResponsesInputItem::Message handling for valid "message" values, and add a
regression test covering a payload with "type": 1 to ensure it fails at the
boundary.
---
Outside diff comments:
In `@crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs`:
- Around line 891-896: The Responses request validator is returning the
Rust-only parameter name instead of the public wire name. Update the
`responses_workflow` validation path that builds
`OpenAiCompatHttpError::invalid_request` for `request.x_context` so
`error.param` uses the canonical `context` name, and make sure the corresponding
handler contract test asserts the exposed parameter name is `context` rather
than `x_context`.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py`:
- Around line 167-183: The E2E check in the response-stream parsing loop is too
weak because it only asserts terminal lifecycle events; tighten it in the
streaming assertion around the events collection so it also requires a
streaming-only text event such as response.output_text.delta or
response.output_text.done, while still keeping the existing response.created and
response.completed ordering checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 162f54ba-b0b4-4173-ac0c-76e51e42cca5
📒 Files selected for processing (7)
crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rscrates/ironclaw_reborn_openai_compat/src/responses.rscrates/ironclaw_reborn_openai_compat/src/responses_workflow.rscrates/ironclaw_reborn_openai_compat/tests/dto_contract.rscrates/ironclaw_reborn_openai_compat/tests/responses_workflow_handlers_contract.rstests/e2e/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.py
| match object.get("type").and_then(serde_json::Value::as_str) { | ||
| Some("message") => { | ||
| #[derive(Deserialize)] | ||
| struct MessageWire { | ||
| role: OpenAiResponsesMessageRole, | ||
| content: serde_json::Value, | ||
| } | ||
| let wire = MessageWire::deserialize(value).map_err(de::Error::custom)?; | ||
| Ok(Self::Message { | ||
| role: wire.role, | ||
| content: wire.content, | ||
| }) | ||
| } | ||
| None if object.contains_key("role") => { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-string type values instead of treating them as legacy messages.
object.get("type").and_then(serde_json::Value::as_str) makes "type"-missing and "type"-present-but-not-a-string indistinguishable. A malformed payload like {"type": 1, "role": "user", "content": "hi"} will therefore hit the legacy role branch and be accepted as OpenAiResponsesInputItem::Message instead of failing at the request boundary. Match on object.get("type") directly so only an actually absent type can use the legacy fallback, and add a DTO regression test for the non-string case. As per path instructions, Fail loud at request boundaries, and as per coding guidelines, Use enums with #[serde(rename_all = "snake_case")] or explicit #[serde(rename = "...")] for fixed small sets instead of string comparisons.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_openai_compat/src/responses.rs` around lines 63 - 76,
The deserialization in responses.rs is treating a present non-string "type" the
same as an absent one, which lets malformed payloads fall back to the legacy
role-based Message path. Update the matching logic in the
OpenAiResponsesInputItem deserializer to inspect object.get("type") directly so
only a truly missing type can use the legacy branch, and reject any non-string
type value before reaching the fallback. Keep the existing MessageWire /
OpenAiResponsesInputItem::Message handling for valid "message" values, and add a
regression test covering a payload with "type": 1 to ensure it fails at the
boundary.
Sources: Coding guidelines, Path instructions
Summary
Validation
tests/e2e/.venv/bin/python -m py_compile tests/e2e/scenarios/test_reborn_webui_v2_legacy_responses_api.pycargo test -p ironclaw_reborn_openai_compat --features openai-compat-beta --test responses_workflow_handlers_contractStacked after #5346.